Skip to content

Conversation

@MartinMueller2003
Copy link
Collaborator

…ace. This resolves a conflict with some other libraries that also use global definitions.

…ace. This resolves a conflict with some other libraries that also use global definitions.
@MartinMueller2003 MartinMueller2003 marked this pull request as draft July 14, 2024 21:13
@MartinMueller2003
Copy link
Collaborator Author

The open questions are:

Is this OK
Should I take it further, Things like the control array are public and they should be made private.

@s00500
Copy link
Owner

s00500 commented Jul 15, 2024

Hm... so this creates a bit of a breaking changes, but I can see the defines are still there ?

I like the idea of addressing the incompatibilities... and I think it is fair enough to go about them like so. I dont think we have any other libraries that have ESPUI as an upstream dependency (not like ArduinoJSON) so I am also less concerned...

As for taking it further, any reason why the control array is public ? I assume nobody uses it in user code right ? Else I would guess it makes sense to go further, any other issues you have in mind here ?

@MartinMueller2003
Copy link
Collaborator Author

I have no idea why it is public. We have accessors to get a control by ID so I do not see a reason for it. The other major breaking change would be to modify the control structure so that its fields are all private. Then force people to use an accessor to modify the values. That will be a significant change to the applications that are taking shortcuts.

As for the #defines, the names are pretty unique and the value is now class specific so the chances for a collision are smaller. If you really want to segregate the code, we could put the ESPUI code into its own name space and then access it with a "using" statement.

Added upgrade instructions.
@MartinMueller2003
Copy link
Collaborator Author

Just added upgrade instructions to the readme and bumped the version to 2.3.1

@s00500
Copy link
Owner

s00500 commented Jul 16, 2024

The ESPUI Verison ? I would rather do that when releasing, not on the merge, but I agree that this should be the next version number.

Shall I release current master as 2.2.4 now to include your upgrades for ArduinoJSON or should we wait till this pr is merged ? (I think a release now makes sense)

@MartinMueller2003
Copy link
Collaborator Author

I would release it as it is. and then we add the breaking change.

@s00500
Copy link
Owner

s00500 commented Jul 16, 2024

did release and updated PIO :-)

@MartinMueller2003
Copy link
Collaborator Author

MartinMueller2003 commented Jul 18, 2024

Update the control management process one level up in OOO. ID and type are now protected by accessors and are now read only.

The management of the list of controls has been gathered into a single file called ESPUIcontrolMgr. This is a singleton factory class responsible for creating, deleting and manipulating controls.

This is about as far as I would want to take things at this time. There are more OOO design patterns that can be applied, but they would be onerous on the existing user base.

@MartinMueller2003 MartinMueller2003 marked this pull request as ready for review July 18, 2024 23:28
@s00500
Copy link
Owner

s00500 commented Nov 26, 2025

Just realized we never merged this.... seems to have slipped my attention... what is the status here ? @MartinMueller2003 still interested in adding this ? I feel like I need to recheck this for what is actually breaking here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants